-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add (initial) support for PHP 8.4 #1631
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Marked as draft as PHP 8.4 for tests might be just a little too early. The deprecation fixes were moved to #1632 so they can be merged already. |
7737dc3 to
2fdbf1e
Compare
2fdbf1e to
ed133ee
Compare
|
@barryvdh I've slightly reworked integration tests to make sure everything passes, LMK what you think! |
ed133ee to
2ca0f85
Compare
|
Why do we need to ignore the platform reqs? |
|
As long as |
|
I think it's tagged now, can we remove the platform reqs skipping? |
| php artisan debugbar:clear | ||
| - name: Check file count in logs | ||
| run: | | ||
| if [ `ls -1q "sample/storage/logs/" | wc -l` -gt 0 ];then exit 1;fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this change exactly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not exactly sure on what the intended check was doing. This check was introduced with #1118 and seems to aim to check for 'succesful completion' of the php artisan debugbar:clear command on a fresh Laravel installation. Here 'succesful' seems to be measured by "not logging any messages" (and not exiting with non-zero code).
However, this metric seems slightly off to me. In this case it was erring, because (due to php-debugbar/php-debugbar#687) deprecations are being logged while running the command. There are ways to work around this, e.g. suppressing deprecations with some Laravel flag.
I've here opted to rather than trying to patch the old definition of 'succes', come up with a new one that actually feature-tests the actual command. Given that the purpose of the command is cleaning json files from the storage/debugbar directory, I've now added a check to ensure the command does just this. Thus the new definition of 'succesful' is now the combination of php artisan debugbar:clear exits with code 0 and does feature-wise what it's expected to do.
2ca0f85 to
e0e449a
Compare
Just rebased and removed the commit, although I'd still argue it might've been beneficial to have for testing either way. |
This MR:
--ignore-platform-req=php+is added tocomposercalls in integration tests, to ignore checking package bounds on upper PHP versionsReplaces deprecated implicit nullable types in parameter declarations (see https://wiki.php.net/rfc/deprecate-implicitly-nullable-types)see Replace implicit nullable types in parameter declarations #1632To make (integration) tests pass, the following changes are introduced:
logsdirectory. Due to deprecations in previous Laravel/Lumen versions being triggered on PHP 8.4, a log file is created by Laravel to log these messages. This MR changes the integration test to solely test functionality of the clear command, by adding a test file to the directory that should be cleared and subequently testing emptiness of that directory.